Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix use of declarations after nested rules (deprecated in Sass 1.77.7) #40623

Merged
merged 1 commit into from
Jul 15, 2024
Merged

Fix use of declarations after nested rules (deprecated in Sass 1.77.7) #40623

merged 1 commit into from
Jul 15, 2024
+3 −3

Conversation

julien-deramond
Copy link
Member

@julien-deramond julien-deramond commented Jul 10, 2024 •
edited
Loading

(Edit: After the creation of this PR, sass@1.77.8 has been released, but it doesn't change anything to this PR that still can be merged as is.)

Description

When using sass@1.77.7, we have some warnings when running npm run css. More info at Sass: Breaking Change: Mixed Declarations.

If we simply move some calls to mixins in our code, the warnings are not displayed anymore. The related files are:

  • scss/_modal.scss
  • scss/_reboot.scss
  • scss/_type.scss

A simple way to measure the possible impact is to compare the built bootstrap.css from the main branch, and this branch:

--- dist/css/bootstrap.css	2024-07-10 18:33:12
+++ /tmp/bootstrap.main.css	2024-07-10 18:32:55
@@ -515,12 +515,12 @@
 legend {
   float: left;
   width: 100%;
   padding: 0;
   margin-bottom: 0.5rem;
-  line-height: inherit;
   font-size: calc(1.275rem + 0.3vw);
+  line-height: inherit;
 }
 @media (min-width: 1200px) {
   legend {
     font-size: 1.5rem;
   }
\ No newline at end of file
@@ -599,68 +599,68 @@
   font-size: 1.25rem;
   font-weight: 300;
 }
 
 .display-1 {
+  font-size: calc(1.625rem + 4.5vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.625rem + 4.5vw);
 }
 @media (min-width: 1200px) {
   .display-1 {
     font-size: 5rem;
   }
 }
 
 .display-2 {
+  font-size: calc(1.575rem + 3.9vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.575rem + 3.9vw);
 }
 @media (min-width: 1200px) {
   .display-2 {
     font-size: 4.5rem;
   }
 }
 
 .display-3 {
+  font-size: calc(1.525rem + 3.3vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.525rem + 3.3vw);
 }
 @media (min-width: 1200px) {
   .display-3 {
     font-size: 4rem;
   }
 }
 
 .display-4 {
+  font-size: calc(1.475rem + 2.7vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.475rem + 2.7vw);
 }
 @media (min-width: 1200px) {
   .display-4 {
     font-size: 3.5rem;
   }
 }
 
 .display-5 {
+  font-size: calc(1.425rem + 2.1vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.425rem + 2.1vw);
 }
 @media (min-width: 1200px) {
   .display-5 {
     font-size: 3rem;
   }
 }
 
 .display-6 {
+  font-size: calc(1.375rem + 1.5vw);
   font-weight: 300;
   line-height: 1.2;
-  font-size: calc(1.375rem + 1.5vw);
 }
 @media (min-width: 1200px) {
   .display-6 {
     font-size: 2.5rem;
   }
\ No newline at end of file
@@ -5504,12 +5504,12 @@
   width: auto;
   margin: var(--bs-modal-margin);
   pointer-events: none;
 }
 .modal.fade .modal-dialog {
-  transform: translate(0, -50px);
   transition: transform 0.3s ease-out;
+  transform: translate(0, -50px);
 }
 @media (prefers-reduced-motion: reduce) {
   .modal.fade .modal-dialog {
     transition: none;
   }
\ No newline at end of file

As it can be seen here, there's no impact as the mixins don't apply values to some siblings CSS rules for the same selector, so the order here is not important and will produce the same CSS bundle in the end.

Target release

This change is retro-compatible as it works with sass@1.77.6 too. However, if we want to help folks, we might envisage to merge this PR, and then maybe release a v5.3.4 rather quickly? What do you think @twbs/css-review?

Type of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the contributing guidelines
  • My code follows the code style of the project (using npm run lint)
  • (N/A) My change introduces changes to the documentation
  • (N/A) I have updated the documentation accordingly
  • (N/A) I have added tests to cover my changes
  • All new and existing tests passed

Live previews

Related issues

Closes #40621

@julien-deramond julien-deramond marked this pull request as ready for review July 10, 2024 16:46
@julien-deramond julien-deramond requested a review from a team as a code owner July 10, 2024 16:46
ffoodd
ffoodd approved these changes Jul 10, 2024
View reviewed changes
Copy link
Member

@ffoodd ffoodd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👌

@francoisdelage
Copy link

+1

@julien-deramond julien-deramond mentioned this pull request Jul 12, 2024
Merged
10 tasks
@bb bb mentioned this pull request Jul 13, 2024
Merged
@simPod
Copy link
Contributor

simPod commented Jul 15, 2024

🚢

@XhmikosR
Copy link
Member

@julien-deramond I'll wait until tomorrow for Dependabot. Then this PR should only include the fix.

@XhmikosR XhmikosR changed the title Bump Sass from 1.77.6 to 1.77.7 Fix use of declarations after nested rules (deprecated in Sass 1.77.7) Jul 15, 2024
@julien-deramond julien-deramond removed the request for review from XhmikosR July 15, 2024 21:08
@julien-deramond julien-deramond merged commit 7f8fed4 into main Jul 15, 2024
14 checks passed
@julien-deramond julien-deramond deleted the main-jd-bump-sass-1.77.7 branch July 15, 2024 21:10
@apepper
Copy link

apepper commented Jul 22, 2024

@julien-deramond Thank you for fixing the deprecation issue of sass!

Is there any ETA when bootstrap version 5.3.4 will be released (which would contain this fix)?

@julien-deramond
Copy link
Member Author

Unfortunately, we don't have a precise release date to communicate, as it'll depend on our availability. But it should be in a few days, hopefully 🙏

@BlueCutOfficial BlueCutOfficial mentioned this pull request Jul 27, 2024
Merged
@markdumay markdumay mentioned this pull request Jul 31, 2024
Open
@daniel-lerch daniel-lerch mentioned this pull request Aug 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ffoodd ffoodd ffoodd approved these changes

@louismaximepiton louismaximepiton louismaximepiton approved these changes

Assignees
No one assigned
Labels
css v5
Projects
v5.3.4
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SASS deprecation warnings with version 1.77.7 - Mixed declarations
7 participants
@julien-deramond @francoisdelage @simPod @XhmikosR @apepper @ffoodd @louismaximepiton